-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add warning for query passthrough #15562
Conversation
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the following:
- Include in all relevant connectors
- Keep it all in the fragment
- Change commit message and PR message to be about the content .. not the mechanics of how you do it (the fragment approach)
40d9d80
to
ce8dd36
Compare
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
Drop the "workaround" from the PR title and commit message please |
ce8dd36
to
a783ca5
Compare
a783ca5
to
d4a2241
Compare
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
d4a2241
to
f2e8be4
Compare
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
8598982
to
e797679
Compare
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
e797679
to
d4463c6
Compare
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
d4463c6
to
0a7f46e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % fact-check
docs/src/main/sphinx/connector/query-passthrough-warning.fragment
Outdated
Show resolved
Hide resolved
You can use passthrough queries to read data; however, queries that write data | ||
typically do not return a table. As a result, when a write operation is passed, | ||
Trino declares that the query failed even if the database performs the | ||
operation. If supported by the underlying data source, you can avoid this | ||
outcome by appending a statement with ``RETURNING 1``. Performing this action | ||
may grant access to restricted data or result in data loss. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this factual? IIRC the query fails during ANALYSIS itself so there's no possibility of a DELETE/UPDATE getting executed if the Trino query shows a failure. @kasiafi can you please double check my understanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hashhar I don't think there's a possibility that an operation (like DELETE or UPDATE) got executed though Trino query failed. I would consider that a serious issue.
All operations that do not return a result set fail during analysis, so they never reach execution.
Please note that the success or failure of a query pass-through depends directly on the presence of a result set, not on the operation type. Theoretically, if some DB returned a result set metadata for a DELETE, we would be happy to execute it. cc @mosabua @Jessie212
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats exactly the point of the warning ... however ... do we know that the query was actually not run on the underlying system .. @electrum was mentioning that maybe a query like a DELETE query is actually passed through .. and run .. and then just fails on the trino side since no result set is returned.. but the query actually deleted records.
So the aim of this PR is to add a warning of whatever the potential behavior is. Could you test and fully verify this @kasiafi and then we can adjust the wording accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the failure happens during analysis of the query. There's no possibility of running a query if it doesn't return a return set. We check whether result set would be returned BEFORE executing the query.
You can verify this behaviour through the various QueryRunners we have (PostgreSqlQueryRunner/MySqlQueryRunner were the ones I verified).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that even possible .. Trino verifies a random pass through query string that is valid for the underlying data source without running the query? Does it pass it through for analysis only or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that even possible .. Trino verifies a random pass through query string that is valid for the underlying data source without running the query? Does it pass it through for analysis only or something? Or does Trino only validate the larger query .. but not the pass through bit .. and if thats the case .. how does it know if that returns a result set or whatever else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should discourage using these table functions for executing queries that might have side-effects in the underlying database. Doing that can be problematic in the face of query or task retries if the operation is not idempotent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we word that so users understand? You are just talking about write access and DDL stuff right? Or any other side effects?
Should we just say that users should use this for just for read-only access?
typically do not return a table. As a result, when a write operation is passed, | ||
Trino declares that the query failed even if the database performs the | ||
operation. If supported by the underlying data source, you can avoid this | ||
outcome by appending a statement with ``RETURNING 1``. Performing this action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all DBs support this syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Postgres has RETURNING
, SQL Server has OUTPUT
. I don't know about others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but yes, this statement won't work as it is for all connectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What connectors support what syntax? Can we get a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats basically the documentation for the underlying database .. its a pass through .. so we cant make a list .. we can only refer to the docs of the data source.
Trino declares that the query failed even if the database performs the | ||
operation. If supported by the underlying data source, you can avoid this | ||
outcome by appending a statement with ``RETURNING 1``. Performing this action | ||
may grant access to restricted data or result in data loss. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove "may grant access to restricted data". If you mean access which would be restricted if the query was run in Trino, then it is equally true for read and write operations.
Regarding "result in data loss", I would reword it. I a user runs a DELETE query, it is their expectation that data will be deleted. "Data loss" sounds like a bug.
0a7f46e
to
9776247
Compare
9776247
to
a133f47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Create fragment and warning callout for query passthrough workaround
Additional context and related issues
Release notes
(s) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: